-
-
Notifications
You must be signed in to change notification settings - Fork 561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix notifications workaround #2051
Conversation
@stkrknds I wanna add this to my fork, |
Sorry, I haven't yet had time to thoroughly test it but the code seems okay. One problem that exists is that only the chats that are loaded in the sidebar would be monitored for new messages. I'm not sure how it would behave when a new message is received but I'll check it out. The ultimate solution would be intercepting the websocket packets that are sent to Messenger website and from it so we could both filter out seen signals and catch new messages as they arrive with much less overhead (compared to monitoring the ui). I started some work in #1688 but it requires completely reverse engineering the websocket protocol they designed so it will definitely take time. |
Hello @Alex313031 . Sure, go ahead. Let me know if you need anything else from me. |
@stkrknds I added it to my fork, and used the lower case y as recommended by @dusansimic |
Once this gets merged, I'll publish a new release. |
If I'm testing this correctly, this only shows a notification once a first new message is received. Once the second one comes in but the first one is not yet read, the notification won't be shown. |
@dusansimic Did you merge this (in modified form) here > Alex313031/caprine-ng@main...sindresorhus:caprine:main I merged @stkrknds commits into my fork, but now mine is diverged too much. Should I remove his, and add the commits of yours, or try to combine them together? |
No, I've added my own commit for lowercase
IMHO, you could just leave it as is for now and merge the final code when we merge it into main. Then you could just revert commit you've made and merge the main branch from upstream. That would be the ideal process for the future, however that would mean the fork will have to wait for changes to land in main on upstream, or at least a green light for merging. |
@stkrknds This patch appears to break on Linux. Whenever someone sends a message, it dings, but then the viewport freezes. The menubar still works, but clicking anywhere in the window does nothing. The only way to resolve it is to relaunch caprine. On Windows it seems to work OK |
I think this is related to #2033. That issue already exists without this patch. I still haven't had enough time to properly investigate the cause of that bug. It could be any number of things that are in Caprine but most likely one is some observer enters a loop because some element gets modified repeatedly. |
Sorry for the late response.
@Alex313031 I don't face this issue with this patch, are you sure that's caused specifically by it?
@dusansimic Yes you are right. I'm trying to make it send notifications for every new message, but it is quite difficult because, taking the worst case scenario(1 emoji & new message identical to the previous one), the only mutation that is observed is the time(next to the message). The issue is that, I don't know when such a mutation can be classified as unread, considering that the time changes even if no new message arrives. |
@stkrknds @dusansimic Reverting the patch makes Caprine work normally again. I made two clones of my repo, and in one, I reverted the commits. I then use npm ci to make sure that the package-lock.json was used, to remove variables. Running the one with the patch leads to caprine freezing. Also, @dusansimic I am aware of #2033 I had this issue too, however, it was resolved until now. |
That is indeed problematic. We could use two things to check if a message is new. If the chat gets promoted up the chat list, it means that it's a new message. And if the text of the message is changed, it's a new message. With those two cases, we could fairly reliably determine if some change in the ui is a new message. There is an extreme case when the new message is the same as the old one and the chat is at the top of the chat list already but I'm guessing we could overlook that for now since that is quite a special case. |
@stkrknds @dusansimic I'm gonna try to bisect and see if I can narrow down the line(s) that are causing this. |
@stkrknds @dusansimic It is definitely something you guys did https://github.com/sindresorhus/caprine/blob/main/source/browser/conversation-list.ts the conversation-list.ts This commit broke caprine > 0d38a71 The other two files are fine. I made two clones of the repo. They were both identical except one had the conversation-list.ts file from the previous revision. That one works fine. The one with the new conversation-list.ts file causes caprine to freeze. IDK how to fix it, but now that you know what it is, maybe yall can figure it out. To rule out environment causing the problem, I did all my testing on a vanilla Ubuntu VM. |
c7eb882
to
08cfb9e
Compare
Made some changes. Needs some more testing. The code is a bit messy, but will clean it later. @Alex313031 I can't reproduce the issue. Don't know if it happens only on specific distros(?). I'm running arch and it's working fine. |
@stkrknds You are running Arch with what DE and WM? And OK I will try it out. |
@stkrknds @dusansimic This seems to fix it! Cool. |
@Alex313031 I'm using the awesomeWM. It's good that it seems to be working fine. I will test it some more over the weekend and hopefully clean the code a bit. |
@stkrknds Yup, once again I can verify that the last commit is what broke it, cloning and building Waiting for the code cleanup, and then will rebase my fork on it and make some releases. My fork is mostly identical, except I enable all features (including "dev only" ones), I made a new "versions" window in the "About" menu item that shows versions of Electron, Chromium, Node, V8, and dependency only node_modules. And my fork is still using Electron 22 - for Windows 7/8/8.1 and Ubuntu 16.04/Debian 8 support. Since Electron 22 is still supported upstream, I feel that most electron apps should keep using it unless there is a specific need for a newer version. (Which there usually isn't since there hasn't been any API changes except for the Mac window button position.) Another minor change that I think I will make a pull request for, is setting the icon explicitly on linux instead of relying only on the packaged version's .desktop file. What I mean is that for Windows and Mac, the icon is set in the code, and will be used even when running |
@stkrknds Also, it DOES appear to be DE/WM specific. Installing Arch and awesomeWM, openBox, or XFCE works fine (I tested in a VM, just to investigate your experience with it.) However, KDE and GNOME (which Ubuntu uses a modified version of) breaks it. Maybe because the former DE's just use libappindicator or xdg-notify, whereas KDE and GNOME have their own DE-specific notifications system. |
@stkrknds @dusansimic How goes the testing on this? Did you guys read me above comments? Will it be merged? I've been using it on Windows and Linux with no problems. |
@Alex313031 Sorry, I'm going to clean the code this weekend. |
bd2f5a9
to
17ca6c3
Compare
Okay, I think it's pretty good now, tested it and it works fine. |
It doesn't seem to work for receiving multiple messages. Also, the contents of the messages is not shown but it might be just on my machine (I run GNOME, but the weird thing is that it worked when I tried it last time). |
@stkrknds Can confirm, seems broken. I hate this cat and mouse chasing with facebook everytime they decide to update their stuff. |
00142e9
to
972ab98
Compare
An update: Right now the only case where notifications are not working is when the same emoji is being sent multiple times continuously. I can't find a way to fix this but I think it is not really that important. I would appreciate it if you could confirm that, with the exception of this particular case, notifications are working as intended. Thanks. |
f018829
to
2468bc0
Compare
source/browser/conversation-list.ts
Outdated
( | ||
mutation.type === 'childList' | ||
&& mutation.addedNodes.length > 0 | ||
&& ((mutation.addedNodes[0] as Element).className === 'x1i10hfl x1qjc9v5 xjbqb8w xjqpnuy xa49m3k xqeqjp1 x2hbi6w x13fuv20 xu3j5b3 x1q0q8m5 x26u7qi x972fbf xcfux6l x1qhh985 xm0m39n x9f619 x1ypdohk xdl72j9 x2lah0s xe8uvvx xdj266r x11i5rnm xat24cr x1mh8g0r x2lwn1j xeuugli xexx8yu x4uap5 x18d9i69 xkhd6sd x1n2onr6 x16tdsg8 x1hl2dhg xggy1nq x1ja2u2z x1t137rt x1o1ewxj x3x9cwd x1e5q0jg x13rtm0m x1q0g3np x87ps6o x1lku1pv x78zum5 x1a2a7pz') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move all the query selectors to the selectors.ts
file and add comments about what exactly they are meant for. It would be far easier to maintain this feature and fix them in case they break.
Other than this, pr looks good.
c4dc96a
to
0ca861a
Compare
@dusansimic Made some changes. Created variables for the selectors and fixed another bug that I found. The freeze issue(#2053) might be caused by the currently broken countUnread function. By fixing this workaround we may indirectly solve the issue. I can't say for sure, since I can't reproduce it. |
@stkrknds I just wanted to push this out quickly so I made a few changes myself. I've moved the selectors to the If everything looks good, we're merging this and I'll push out a release today. |
Works fine. |
Thanks for the help! |
I had no notifications so far. Does it look for english text or something? Facebook appears localized in Caprine for me. |
@mzso No, the language shouldn't matter. Right now there are some selectors that need to be updated, so notifications may not work. Did you use the version 2.59 a week ago? I think notifications worked then. |
@stkrknds I think I used it as soon as it came out. |
They did, the selectors seem to be broken. |
It seems to be working now. At least I saw the tray icon turn purple a couple times. |
Since Messenger does not send notifications, the use of this workaround is necessary. This pull request fixes/updates the workaround in order to work with the new design. I've been testing it for a few weeks and it works fine. However, I would be grateful if someone else could test it as well and report back. Thanks in advance!
Fixes #1562
Fixes #1825